-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 Sanitize temporary filename for podman image load #803
🐛 Sanitize temporary filename for podman image load #803
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #803 +/- ##
=======================================
Coverage 67.21% 67.21%
=======================================
Files 23 23
Lines 1467 1467
=======================================
Hits 986 986
Misses 415 415
Partials 66 66
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Makefile
Outdated
rm $(IMG).tar | ||
$(CONTAINER_RUNTIME) save $(IMG) -o /tmp/$(shell echo $(IMG) | sed 's/[/:]/-/g').tar | ||
$(KIND) load image-archive /tmp/$(shell echo $(IMG) | sed 's/[/:]/-/g').tar --name $(KIND_CLUSTER_NAME) | ||
rm /tmp/$(shell echo $(IMG) | sed 's/[/:]/-/g').tar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use mktemp
here? I think it will be a bit more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just with the default randomly generated filename to simplify all of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or even just a shell variable for the filename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, yeah good idea lol
Makefile
Outdated
@@ -156,9 +156,9 @@ e2e-coverage: | |||
kind-load: $(KIND) #EXHELP Loads the currently constructed image onto the cluster. | |||
ifeq ($(CONTAINER_RUNTIME),podman) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably out of scope for what this PR is trying to accomplish (and I asked in the other now-closed PR), but would be possible to use this podman procedure for docker
as well? That way we don't have to maintain two different implementations of kind-load.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be find doing that, I can just remove the ifeq bits and leave only the first block with the temp archive. That should always work regardless of if the user is using podman, docker, or podman with the "podman-docker" alias package.
62017c9
to
5ee07f7
Compare
Makefile
Outdated
TMP_FILE=$(shell mktemp -t operator-controller-image-archive-XXXX.tar) ;\ | ||
$(CONTAINER_RUNTIME) save $(IMG) -o $$TMP_FILE ;\ | ||
$(KIND) load image-archive $$TMP_FILE --name $(KIND_CLUSTER_NAME) ;\ | ||
rm $$TMP_FILE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even simpler?
$(CONTAINER_RUNTIME) save $(IMG) | $(KIND) load image-archive /dev/stdin --name $(KIND_CLUSTER_NAME)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice, implemented in latest push
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you or someone else has docker available, would be nice just to double check this works as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh but the CI probably uses docker doesn't it?
This simplified logic should work on both podman and docker, removing our need to have distinct ways of handling the image loading.
5ee07f7
to
d813099
Compare
Description
The podman-specific kind-load commands in the Makefile currently write the image archive to a temporary file named $IM but that by default includes slashes and a colon. My changes replace any slashes or colons with hyphens to prevent any errors trying to write that file.
Also, the naming doesn't really matter, we can just make it be a hard-coded "tmp.tar" or something.
Also, also, I saw the closed PR from earlier today and the discussion about not getting too into the weeds on podman/docker-specific configuration in the Makefile, but the current setup will cause that file-write error for anyone who does use CONTAINER_RUNTIME=podman with our default env variables, so I figured that was worth a quick fix.
Reviewer Checklist